Skip to content

docs: clarify dual-memory architecture (fixes #344)#367

Open
jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
jlin53882:docs/dual-memory-ux-344
Open

docs: clarify dual-memory architecture (fixes #344)#367
jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
jlin53882:docs/dual-memory-ux-344

Conversation

@jlin53882
Copy link
Copy Markdown
Contributor

Summary

Addresses Issue #344: Dual-memory model causes confusing UX — users expect Markdown memories to be recallable, but they aren't.

Adds three improvements:

1. README: Dual-Memory Architecture section

Explains the distinction between Plugin Memory (LanceDB) and Markdown Memory, with a clear table and usage guidance.

2. index.ts: Startup warning log

Logs a human-readable warning on every plugin initialization.

3. cli.ts: import-markdown command


px memory-lancedb-pro memory-pro import-markdown\ — migrate Markdown memories into the plugin store for semantic recall.

Test results

Gateway log after restart:
\
01:23:46 [memory-lancedb-pro] memory_recall queries the plugin store (LanceDB), not MEMORY.md...
01:23:49 memory-lancedb-pro: initialized successfully (embedding: OK, retrieval: OK, mode: hybrid, FTS: enabled)
\\

Closes #344

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dcdb9b8cf8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

index.ts Outdated
Comment on lines +1670 to +1674
if (_initialized) {
api.logger.debug("memory-lancedb-pro: register() called again — skipping re-init (idempotent)");
return;
}
_initialized = true;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Defer init flag until registration succeeds

Setting _initialized = true before parsing config and creating runtime components makes registration non-recoverable: any thrown error on first startup leaves the module permanently “initialized,” so later register() calls silently no-op. In environments that retry plugin startup (or reload after fixing config), the plugin stays disabled until process restart.

Useful? React with 👍 / 👎.

index.ts Outdated
// recallMode: "full" (default when autoRecall=true) | "summary" (L0 only) | "adaptive" (intent-based) | "off"
const recallMode = config.recallMode || "full";
if (config.autoRecall === true && recallMode !== "off") {
if (config.autoRecall === true) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Respect recallMode when deciding auto-recall enablement

This condition now enables auto-recall whenever autoRecall === true, but the previously supported recallMode: "off" path is no longer checked. Existing configs that keep autoRecall on but set recallMode to disable injection will now inject memories unexpectedly, changing prompt contents and behavior for deployed users.

Useful? React with 👍 / 👎.

Comment on lines 1711 to 1714
model: config.embedding.model || "text-embedding-3-small",
baseURL: config.embedding.baseURL,
dimensions: config.embedding.dimensions,
omitDimensions: config.embedding.omitDimensions,
taskQuery: config.embedding.taskQuery,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Forward omitDimensions into embedder config

The embedder is constructed without omitDimensions, so users who configured embedding.omitDimensions=true lose that behavior and will start sending dimensions again. For OpenAI-compatible backends that reject the dimensions field, embedding calls fail, which breaks memory writes and retrieval flows relying on embeddings.

Useful? React with 👍 / 👎.

@jlin53882
Copy link
Copy Markdown
Contributor Author

Fixes Applied for Your 3 P1 Review Issues

Applied all 3 fixes directly to this PR:

P1 #1: Defer _initialized until after setup succeeds

Fixed by moving _initialized = true from the beginning of register() to the end ??only set after all components initialize successfully. This makes the plugin recoverable if init fails partway through.

P1 #2: Restore omitDimensions forwarding

Fixed by re-adding omitDimensions: config.embedding.omitDimensions to the createEmbedder() call. Without this, users with embedding.omitDimensions: true would fail on OpenAI-compatible backends that reject the dimensions field.

P1 #3: Restore recallMode check in auto-recall gate

Fixed by restoring:

const recallMode = config.recallMode || full;
if (config.autoRecall === true && recallMode !== off) {

This prevents unexpected memory injection for users who set recallMode: off with autoRecall: true.

All fixes are in the latest commit. Thanks for the detailed review @chatgpt-codex-connector[bot]!

@chatgpt-codex-connector
Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the dual-memory architecture docs — the table and explanation are clear and much needed! One scoping concern before we merge:

The cli.ts change adds a new import-markdown command (125 lines) — that's a code feature, not documentation. Could you split it into a separate feat: PR so it can get proper review of the CLI error handling and file path resolution? The README addition and startup warning log can merge as-is.

Also noticed this PR and #356 both delete the same symbols from index.ts (runCompaction, normalizeAutoCaptureText, etc.) — these will conflict at merge time. Is there a shared refactor commit underlying both? If so, it would be cleaner to land that as a dedicated PR first.

Add two improvements addressing Issue CortexReach#344:

1. README: add Dual-Memory Architecture section explaining
   Plugin Memory (LanceDB) vs Markdown Memory distinction
2. index.ts: log dual-memory warning on plugin startup

Refs: CortexReach#344
@jlin53882 jlin53882 force-pushed the docs/dual-memory-ux-344 branch from 738ad76 to 389750c Compare March 31, 2026 10:53
@jlin53882
Copy link
Copy Markdown
Contributor Author

PR Split — closed by this update

This PR has been split into two:

PR Contents
#367 (this PR) README + startup warning (docs only)
#426 import-markdown CLI command (feat)

Changes in this update:

@AliceLJY please review #367 when ready — it should be clean to merge now.

@jlin53882
Copy link
Copy Markdown
Contributor Author

PR Split Update

This PR has been split into two separate pull requests:

PR Title Contents
#367 (this PR) clarify dual-memory architecture README + startup warning (docs only)
#426 feat: add import-markdown CLI command import-markdown CLI command

Changes in this update

  • Removed cli.ts — moved to PR feat: add import-markdown CLI command #426 as a standalone feature PR
  • Rebased onto latest master — no longer deletes any existing functionality (runCompaction, normalizeAutoCaptureText, etc. are all preserved)
  • Only adds:
    • README.md: Dual-Memory Architecture explanation section
    • index.ts: startup log warning about the two-layer memory model

Related PRs

PR Status Description
#307 Awaiting close autoRecallExcludeAgents — covered by #365
#356 Pending rerankTimeoutMs base commit done; reviewer feedback pending
#365 Ready idempotent guard + governance logging + autoRecallExcludeAgents (ported from #307)
#367 Ready (this PR) README + startup warning
#426 Ready import-markdown CLI

@AliceLJY please review #367 when ready — it should be clean to merge now.

@AliceLJY
Copy link
Copy Markdown
Collaborator

Thanks for splitting this out and rebasing — the dual-memory architecture section is clear and will help users a lot.

A few things before we can merge:

  1. cli.ts deletion: The diff still shows cli.ts being fully deleted (1349 lines). Your comment says this PR "no longer deletes any existing functionality," but if cli.ts exists on master, this would remove all CLI commands (search, stats, login, upgrade, etc.), not just import-markdown. Could you double-check whether this is a rebase artifact or an unintended inclusion?

  2. import-markdown reference in README: The new docs section includes npx memory-lancedb-pro memory-pro import-markdown, but since that command now lives in PR feat: add import-markdown CLI command #426, users reading these docs after merge would see a non-functional command. Consider removing it from this PR and adding it when feat: add import-markdown CLI command #426 lands.

  3. Unrelated README removals: The diff removes the "Auto-recall timeout tuning" FAQ and the beta.10 version banner — was that intentional?

The dual-memory table and startup log look great as-is. Once these items are resolved this should be ready to go! 🙏

jlin53882 added a commit to jlin53882/memory-lancedb-pro that referenced this pull request Apr 1, 2026
…own reference, restore removed README sections

- Restore cli.ts (was accidentally deleted, all CLI commands preserved)
- Remove import-markdown command reference from dual-memory section (lives in PR CortexReach#426)
- Restore beta.10 version banner and OpenClaw 2026.3+ badge
- Restore Auto-recall timeout tuning FAQ section

Ref: CortexReach#367
…own reference, restore removed README sections

- Restore cli.ts (was accidentally deleted, all CLI commands preserved)
- Remove import-markdown command reference from dual-memory section (lives in PR CortexReach#426)
- Restore beta.10 version banner and OpenClaw 2026.3+ badge
- Restore Auto-recall timeout tuning FAQ section

Ref: CortexReach#367
@jlin53882
Copy link
Copy Markdown
Contributor Author

Hi @AliceLJY — all review items have been addressed in the latest push:

  1. cli.ts deleted — restored from upstream/master (all CLI commands preserved)
  2. import-markdown reference — removed from dual-memory section (lives in PR feat: add import-markdown CLI command #426)
  3. beta.10 banner + OpenClaw 2026.3+ badge — restored
  4. Auto-recall timeout tuning FAQ — restored

Please re-review when you have a moment 🙏

Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — review feedback addressed cleanly. cli.ts restored, import-markdown split out, docs-only scope now.

Assigning to @rwmjhb for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify source-of-truth between Markdown memory and plugin recall (dual-memory model causes confusing behavior)

3 participants